fix(tern): settle sequential tasks when the engine loses in-flight work - #1113
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens the sequential Tern drive against “lost in-flight work” scenarios where the engine reports no active schema change while storage still shows a task running, which previously could lead to silent infinite polling and a wedged apply.
Changes:
- Adds bounded “lost work” detection in the sequential poll loop and verifies target convergence via a re-plan when the pending/no-active signal persists.
- Introduces a stall watchdog that rate-limits warnings when task state/progress does not move for a configured interval.
- Expands sequential progress tests to cover lost-work settlement (converged vs not), stale-snapshot self-heal, and watchdog warning behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/tern/local_client.go | Adds LocalClient overrides for sequential poll cadence and stall-warning interval (primarily for tests). |
| pkg/tern/local_apply_sequential.go | Implements lost-work tolerance + verification, bounded error handling integration, and a stall watchdog with rate-limited warnings. |
| pkg/tern/local_apply_sequential_progress_test.go | Adds fixtures and tests for lost-work settlement paths and watchdog logging/rate limiting. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The sequential drive's progress poll tolerates a short window of the engine reporting no active schema change for an in-flight task (a stale snapshot after an engine restart self-heals), then stops trusting the engine and verifies the target schema directly: a converged target completes the task through the normal completed flow, a target that still needs the change marks the task retryable so a fresh claim re-drives it, and verification errors count against the poll's bounded consecutive-error budget. Every branch logs with the task's triage attributes and the engine-reported state. The poll also carries a stall watchdog: a task sitting in the same state with unchanged progress fields for a full interval is warned about once per interval, without changing task state. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
e950fc7 to
9f7610a
Compare
…nd trust the engine on a clock A task in its revert phase is in flight, so the sequential drive's lost-work verification could reach it. Reading the target schema cannot settle a revert: the forward change has already cut over, so the live schema matches the reviewed target by definition and a match says nothing about whether the revert ever finished. That path reported the apply as a successful schema change while the revert it was undoing was gone. A revert-phase task is now marked retryable without consulting the target, matching the guard the resume path already applies for the same reason. The trust budget before that verification runs is now a duration rather than a count of polls. What it has to outlast is wall-clock engine behaviour, not a number of round trips: an engine that just restarted serves a stale snapshot until it catches up, and an engine whose remote work is still being provisioned or validated reports no active schema change for real, healthy work it has not begun executing. A poll-count budget silently shrinks to nothing whenever the poll cadence is shortened, which bounced a healthy apply to retryable and re-drove it — worse than waiting on an engine that provisions remote resources per attempt. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… an operator gate A held cutover, a deferred deploy and an open revert window are all states the drive is meant to sit in without moving, because the next step belongs to an operator rather than to the engine. Progress fields do not advance there by design, so the stall warning fired once per interval for as long as the operator took to act — on every deferred cutover and deferred deploy. The watchdog still observes every poll, so entering or leaving one of these states restarts its clock and a genuinely stuck task still warns. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The trust budget before the drive verifies the target schema was one constant for every engine, sized for the slowest of them. That is the wrong shape: how long a pending progress report stays ambiguous is a property of the engine, not of the drive. An engine that provisions after accepting work — cutting a branch, opening and validating a deploy request — reports pending for real, healthy work for as long as that setup takes, so a driver must give it time. An engine that publishes its tracked schema change before Apply returns has no such phase: once Apply has returned the work is either running or it is gone, so the first pending report about an in-flight task is already conclusive and there is nothing to wait out. A new optional engine.SynchronousWorkRegistration lets an engine declare which it is, and the sequential drive reads that declaration to size the budget. Spirit declares it: it runs in a goroutine of this process with nothing to provision, and Drain and Cancel are the only writers that clear the tracked state, both of which mean the work is not coming back. Engines that do not declare it keep the full budget, which is the safe default — provisioning is never mistaken for lost work. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
🤖 Review findings - created by Kiran's code review agent - for schemabot/pull/1113, 7882487. Verdict: 7 findings — 3 non-blocking (sibling drive still hangs, untested settle-error branch, comment/constant drift), 4 general suggestions. Non-blocking1. The grouped drive has the identical unbounded-pending hang, and Spirit reaches it on every
return apply.DatabaseType == storage.DatabaseTypeMySQL && storage.ApplyOptionsFromMap(options).DeferCutover
Failure scenario: a MySQL 2. The failed-verification branch — including the bound the comment says "must never become an unbounded loop" — has no test. if consecutiveErrors >= maxConsecutiveProgressPollErrors {
c.markTaskRetryable(ctx, task, fmt.Sprintf("engine reports no active schema change for an in-flight task and target verification failed after %d consecutive errors; see server logs", consecutiveErrors))
3. The new constant's doc comment asserts a coupling to the grouped poll that does not exist in code. // maxConsecutiveProgressPollErrors bounds how many consecutive progress
// poll failures the sequential drive tolerates before settling the task,
// matching the grouped poll.The grouped poll keeps its own literals — General suggestions4. The Postgres engine qualifies for e.claimProgress(key, progressResult(engine.StateRunning, "preflight", started, change, ""))runs synchronously before 5. Repeated verification failures re-plan the whole database once per 500 ms tick with no backoff. action, settleErr := c.settleLostEngineWork(ctx, apply, task, result.State)
if settleErr == nil {
return action
}On error the loop 6. The stall warning carries no throttle context, so an engine-paced copy reads as a wedged one. if stalledFor, warn := watchdog.observe(now, taskProgressSnapshotOf(task)); warn && !taskWaitsForOperatorAction(task.State) {
7. Two verdicts now exist for the same physical condition. if result.Message == "No active schema change" {The conflict check marks that task The one thing that could have broken, verifiedSpirit declaring I proved it safe three ways:
I also tried and failed to build a lease-loss variant of that race: a drive whose lease is lost or presumed lost has its context cancelled by Verified correct
This review was generated by Claude Code (claude-opus-5). |
…ost-work verification The PostgreSQL engine claims its tracked progress under the engine mutex before Apply returns and only Drain clears it, so a pending report about in-flight work is conclusive; declaring engine.SynchronousWorkRegistration lets a drive verify the target immediately instead of spending a trust budget the engine does not need. The grouped poll now shares the sequential drive's poll-interval and error-budget constants, and the retryable message for an unverifiable target attributes the error count to progress polls and target verification together rather than to verification alone. A new test pins that failed verification reads are bounded by the shared error budget and rest the task retryable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A throttled task and a wedged one look identical to the stall watchdog: no state or progress movement past the warning interval. Adding the engine's throttle flag and reason to the warning answers the first triage question — is the engine deliberately holding back — from the log line alone. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
🤖 Thanks — addressed at 0cd1acd + b8a9c9e, finding by finding:
Each change was mutation-verified: flipping the Postgres declaration to false, downgrading the exhausted-budget branch from retryable to failed, and dropping the throttle attributes each fail at least one test on re-run. This reply was generated by Claude Code (Claude Fable 5). |
# Conflicts: # pkg/engine/postgres/postgres_test.go
…t-dialect-classify * origin/main: fix(github): keep the PR progress comment updating between operation dispatch waves (#1104) fix(tern): classify materialized change DDL with the target dialect parser (#1187) fix(engine): resolve a cancel or stop that arrives before remote dispatch (#1184) fix: default connect and write timeouts on managed database connections (#1182) fix(storage): index the apply-operation claim ordering (#1180) fix(tern): generalize control resume state and complete cancels with no live engine work (#1179) fix(github): name each table's outcome in unsuccessful apply summaries (#1186) ci: peel tern and webhook into a dedicated integration shard (#1166) fix(engine): report a drained schema change's terminal outcome instead of pending (#1114) feat(serve): contain gRPC handler panics with recovery interceptors (#1164) feat(observability): tell operators when a log window hides older entries (#1185) fix(tern): settle sequential tasks when the engine loses in-flight work (#1113) fix(github): align PR comment severity glyphs with the shared vocabulary (#1135) fix(tern): release a database held by a stopped schema change (#1175) fix(plan): canonicalize drift DDL with the target's dialect parser (#1177) fix(e2e): stop injecting connection kills once the k8s pause is observed (#1178) # Conflicts: # pkg/webhook/templates/plan.go
…lassify' into kiran01bm/apply-comment-dialect * origin/kiran01bm/plan-comment-dialect-classify: fix(github): line-break non-MySQL DDL, schema labels for postgres fix(github): keep the PR progress comment updating between operation dispatch waves (#1104) fix(tern): classify materialized change DDL with the target dialect parser (#1187) fix(engine): resolve a cancel or stop that arrives before remote dispatch (#1184) fix: default connect and write timeouts on managed database connections (#1182) fix(storage): index the apply-operation claim ordering (#1180) fix(tern): generalize control resume state and complete cancels with no live engine work (#1179) fix(github): name each table's outcome in unsuccessful apply summaries (#1186) ci: peel tern and webhook into a dedicated integration shard (#1166) fix(engine): report a drained schema change's terminal outcome instead of pending (#1114) feat(serve): contain gRPC handler panics with recovery interceptors (#1164) feat(observability): tell operators when a log window hides older entries (#1185) fix(tern): settle sequential tasks when the engine loses in-flight work (#1113) fix(github): align PR comment severity glyphs with the shared vocabulary (#1135) fix(tern): release a database held by a stopped schema change (#1175) fix(plan): canonicalize drift DDL with the target's dialect parser (#1177) fix(e2e): stop injecting connection kills once the k8s pause is observed (#1178) # Conflicts: # pkg/webhook/templates/apply.go
What breaks today
The loop driving one table's schema change polls the engine until it reports the work finished or failed. That is the loop's only exit — so if the engine forgets the work, there is no exit.
Engines do forget. Their view of in-flight work lives in one process's memory, and it vanishes when that process restarts, or when a sibling drive on the same pod drains the engine in the window between the work finishing and the next poll. From then on the engine answers "no active schema change", with no error, forever — which reads exactly like a stale poll, and the loop is built to ride stale polls out.
Nothing notices, because from the outside nothing is wrong: heartbeats stay healthy, the error budget never burns, recovery never fires. Meanwhile the apply holds the database's one active-apply slot, so every later apply to that database queues behind work that will never finish.
The fix
When the engine reports no active work but stored state says the work is in flight, ask the target database — the one authority that cannot forget an outcome.
The catch is that "no active work" is overloaded. An engine that provisions after accepting work — cutting a branch, validating a deploy request — says the same thing about real, healthy work that simply hasn't started yet, and bouncing one of those re-drives an apply that was about to run fine. So engines declare how to read their own answer, through a new optional
engine.SynchronousWorkRegistration. Spirit and the PostgreSQL engine declare it: both run the work in this process with nothing to provision, so onceApplyhas returned the work is either running or gone and the first such report is conclusive. Undeclared engines are assumed to provision and get a bounded wait first, which is the safe default.That wait is a duration rather than a count of polls, because what it has to outlast is engine behaviour measured in wall-clock time; a poll count would silently shrink to nothing if someone shortened the poll interval.
The revert case is the sharp edge. After a forward change cuts over, the live schema matches the reviewed target by definition — so a match says nothing about whether the revert this task was driving ever finished, and completing on it would report a successful schema change while the revert is gone. A revert-phase task settles retryable without reading the target at all, the same guard the resume path already applies. Beyond that, verification can only ever land
failed_retryable, neverfailed, and errors reading the target burn the existing consecutive-error budget so this path can't become a second silent loop.Also: the drive says when it's stuck
The same loop gains a stall watchdog. When a task's state and progress counters haven't moved for the warning interval, it logs once per interval with the task's triage attributes, how long it's been motionless, and what the engine reports. It observes only — it never changes task state. Tasks parked at an operator gate (held cutover, deferred deploy, open revert window) are motionless by design and stay quiet, and since the watchdog still sees every poll, entering or leaving a gate restarts its clock.
Engine progress is a display feed; outcomes belong to durable state. This is the first drive path where the target database settles a task when the engine's in-memory view and stored state disagree — progress polls decide what operators see, durable state and the target decide what is true. The capability interface follows the precedent
ExternallyAuthoritativeProgressset: instance-local memory is never trusted as truth unless an engine explicitly says it can be.PR summary written by Claude Code (Opus 5).